This effectively resolves the portID and channel ID into a counterparty. So the portID and channelID are in effect the clientID here. It's just a different arbitrary string identifier.

The one annoying thing is that the portID here is both acting as part of the v2 "client-id" (though we could move away from this with a different storage format since channel ids are unique) and also selecting the application to use.

This is fine for packets that are only sending one app data.

For MULTI_PACKET_DATA, we can deal with this by leaving the top portID as is, with the plan to eventually deprecate it. And then still allowing different ports in the internal packet data list

If we have a transfer V2 packet sent from an ibc-go v10 chain to another ibc-go v10 chain over an existing channel, the packet's DestinationChannel will be filled in with the client ID of the destination chain, right? But then in the OnRecvPacket callback of the transfer the hash of the denomination will not match that of the tokens received previously (since those used the destination channel ID and not a client ID). Then those vouchers would not be fungible, right?

No, the DestinationChannel will continue to be the existing channel identifier (that's the purpose of this adjustment, to keep fungibility but allow existing channels to send v2 packets)

Think it'd be nice to see this Counterparty type as something used by historical situations (channel/port), but not necessarily only for that

similar naming suggestion

byte slice needs channel id info?

think this comment is stale from when Aditya wanted to mush these together?

my understanding is that we basically want logic:

if channeltypes.IsChannelIDFormat(sourceChannel) {
    counterparty, ok = k.channelKeeper.GetV2Counterparty(ctx, sourcePort, sourceChannel)
} else {
    counterparty, ok = k.GetCounterparty(ctx, sourceChannel)
}
if !ok {
    return 0, errorsmod.Wrap(types.ErrCounterpartyNotFound, sourceChannel)
}

I think this can be removed now. I see this mapping as:

key: my identifier (v1: channel/port, v2: clientid)
value: counterparty id, merkle path

The client id used to verify the counterparty is in the key, or can be resolved in the v1 case. It seems redundant atm, I would guess in the actual implementation it would only be referenced by the grpc, but we should probably create a new struct for grpc, like we did with the other types IdentifiedClientState for example

really we have a mapping from: our reserved key path -> counterparty reserved key path

resolving the client id responsible for the key path, should happen outside of this mapping?

Before I forget, I went down the thought rabbit hole on this. The proposed construction works perfectly fine, but there's just some minor considerations one could have:

The current construction embeds the clientID into the Counterparty. When we handle v1 cases, the clientID is resolved and set into the counterparty type. In the v2 case, the value is redundant as it is in the key setting the value. The main benefit is code structure as it bundles all the necessary components into a single type. One could also return the clientID in the get counterparty function to achieve the same functionality.

The main reason to keep this approach, would be for future scenarios where we delete the channel/connection mappings and you want to migrate such that all v1 mappings are directly embedded into the counterparty type. One could have two mappings, as I hint at in the above comment. One to obtain the counterparties keyspace which is set for all v1/v2 and one to obtain the light client either directly in v2 case, or via a lookup in v1 case.

The reason why I went this rabbit hole was primarily due to naming. counterparty.clientID referring to your local client identifier, and not a client identifier on the counterparty, could be confusing. Alternatively, if you renamed the Counterparty to something like Connection or Channel, then connection.clientId and connection.counterpartyID could make sense. Either way, just a minor consideration.